-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(types): change attributes type to 'object | undefined' #13467
Conversation
change attributes type to 'object': - attributes param in QueryInterface.bulkInsert() change to 'object | undefined' from 'string[] | string | undefined'. - attributes param in QueryInterface.bulkUpdate() change to 'object | undefined' from 'string[] | string | undefined'. - add '{}' as last param in bulkInsert() & bulkUpdate() tests Closes sequelize#13466
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, why are you using the query interface directly? Is this correct on the model definition but not queryInterface?
types/lib/query-interface.d.ts
Outdated
@@ -498,7 +498,7 @@ export class QueryInterface { | |||
tableName: TableName, | |||
records: object[], | |||
options?: QueryOptions, | |||
attributes?: string[] | string | |||
attributes?: object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't right either actually.
I don't know if there's already a type for this or not. At the top of my head, it should be moreso like:
type Attribute = Literal | string | [Literal | Fn | Col , string]
type Attributes = Array<Attribute> | { include?: Array<Attribute>, exclude: Array<Attribute> }
note: this is mostly psuedocode, I don't remember the exact typings for col, fn, and literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allawesome497 Thanks for the suggestions and you are right. It isn't correct either. I think ModelAttributes
from Model.d.ts seems to be appropriate here. What do you say?.
sequelize/types/lib/model.d.ts
Line 1358 in 23aa67e
export type ModelAttributes<M extends Model = Model, TCreationAttributes = any> = { |
I might be wrong here, But my suggestion are deduced from its usage in the following methods:
bulkInsertQuery(tableName, fieldValueHashes, options, fieldMappedAttributes) { |
updateQuery(tableName, attrValueHash, where, options, attributes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such as this?
type Attribute<
ModelAttr extends string
> = Literal | ModelAttr | [Literal | Fn | Col, ModelAttr];
type Attributes<
ModelAttr extends string = string
> = Array<
Attribute<ModelAttr> | {
include?: Array<Attribute<ModelAttr>>,
exclude:? Array<Attribute<ModelAttr>>,
}
>;
In usage:
attributes?: Attributes<keyof ModelAttributes>
If so, then I think that would be the ideal solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allawesome497 Sorry, I don't understand why do we need { include?: Array<Attribute<ModelAttr>>, exclude:? Array<Attribute<ModelAttr>>, }
and in what scenario Attribute
could be Literal | ModelAttr | [Literal | Fn | Col
?.
Based on the usage in QueryInterface.BulkUpdate
& QueryInterface.BulkInsert
type of attributes should:
attributes?: ModelAttributes<any, any>
ModelAttributes
interface is already defined in model.d.ts
sequelize/types/lib/model.d.ts
Line 1361 in 5e9c209
export type ModelAttributes<M extends Model = Model, TCreationAttributes = any> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So { include?: Array<Attribute<ModelAttr>>, exclude:? Array<Attribute<ModelAttr>>, }
is needed because you can specify those instead of an array. It's mentioned here, though it doesn't go into depth about it.
in what scenario
Attribute
could beLiteral | ModelAttr | [Literal | Fn | Col]
So sequelize allows the following attributes:
col
: the name of a column. Might not be accepted in sequelize, not sure.fn
: A function that is to server as an attribute. Say,fn('COUNT', col('my_column'))
for example. This might need to have an aliasliteral
: Literal sql to be injected as an attribute. I believe this needs an alias specified, so it would only be valid in that tuple.- [item, alias]: this is translated to
${item} AS ${alias}
.
I could be wrong on a few things here though, since this is all from memory. I'm confident on fn and literal being usable in attributes though, at least with an alias specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I've recently seen another type that I didn't realized existed - SequelizeMethod
. This is going to be what we want, rather than Literal | Fn | Col
as those functions extend this.
- In BulkUpdate function update attributes type to ModelAttributes. - In BulkInsert function update attributes type to ModelAttributes.
We merged #13945 a little while ago which updated bulkInsert. I'm pretty sure the new typings for bulkInsert are correct and should be an object instead of an array based on their usage in query-generator.js bulkUpdate hasn't been touched by that other PR however based on their usage in If you're still willing to update this pretty old PR (sorry about that), I can take a look into merging this |
Looks like this has since been fixed by another. Thank you for opening this one nonetheless. Sorry we never got around to merging it :) |
Pull Request check-list
Please make sure to review and check all of these items:
npm run test
ornpm run test-DIALECT
pass with this change (including linting)?Description of change
change attributes type to 'object':
Closes #13466